Add `forked_from` as a pointer either to an existing package or as a reference to a URL #3363

daveverwer · 2024-09-05T06:22:30Z

Supercedes #3355 and implements the first part of #551. UI changes still pending.

@supersonicbyte I was seeing test failures in test_decode_Metadata_null and test_fetchMetadata which I have fixed by making Parent optional in the GitHub metadata. I don’t believe that parent is a mandatory field supplied by GitHub, so this feels like a reasonable fix to me but could you verify please?

Thanks!

finestructure · 2024-09-05T07:05:29Z

Yes, parent is optional in the GraphQL schema:

Note that GraphQL uses sort of a reverse syntax to Swift:

optionalField: String          -> var optionalField: String?
nonOptionalField: String!      -> var nonOptionalField: String

finestructure · 2024-09-05T07:09:37Z

Sources/App/Models/Repository.swift

    var summary: String?
+
+    @Field(key: "forked_from")
+    var forkedFrom: Fork?


Nit: we're trying (😅) to keep the data fields sorted (although this isn't true generally, because it's something we started half-way through without going back and fixing it everywhere). I'll patch this up real quick.

supersonicbyte

LGTM! Thanks for the review and fixes. @finestructure could you link the logic for normalizing the repo urls from PackageList? I couldn't find it.

finestructure · 2024-09-05T07:30:34Z

Sources/App/Commands/Ingest.swift

+        }
+    } catch {
+        Current.logger().warning("updating forked from failed")
+    }


I'd rather we didn't stick this in updateRepository like this - the function should really only update repository from parameters passed in (even if the "computation" is rather trivial) and shouldn't have other side effects like running a package query.

Instead, let's make updateRepository take a parameter:

func updateRepository(on database: Database, for repository: Repository, metadata: Github.Metadata, licenseInfo: Github.License?, readmeInfo: Github.Readme?, s3Readme: S3Readme?, fork: Fork?) async throws {

and move the "fork logic" to

func getFork(on database: Database, metadata: Github.Metadata) async throws -> Fork? { guard let parentUrl = metadata.repository?.normalizedParentUrl else { return nil } if let packageId = try await Package.query(on: database) .filter(\.$url, .custom("ilike"), parentUrl) .first()?.id { return .parentId(packageId) } else { return .parentURL(parentUrl) } }

which we then use around line 128 before the updateRepository call:

let fork = try await getFork(on: database, metadata: metadata) try await updateRepository(on: database, for: repo, metadata: metadata, licenseInfo: license, readmeInfo: readme, s3Readme: s3Readme, fork: fork)

This will also allow us to add a couple of tests specifically for getFork behaviour like

returning nil on normalisation failure

correct matching when urls differ in case

covering both the url and the id based lookups

finestructure · 2024-09-05T08:01:29Z

Tests/AppTests/IngestorTests.swift

+            .filter(\Repository.$package.$id == forkedId).first()
+
+        XCTAssertNil(repo?.forkedFrom)
+    }


By moving the main functionality to getFork, these tests become much easier to set up because we don't need to test all of ingest. Instead we just test getFork in isolation:

func test_getFork() async throws { try await Package(id: .id0, url: "https://github.com/foo/parent.git".url, processingStage: .analysis).save(on: app.db) try await Package(url: "https://github.com/bar/forked.git", processingStage: .analysis).save(on: app.db) var metadata = Github.Metadata.mock do { // test lookup when package is in the index metadata.repository?.parent = .init(url: "https://github.com/foo/parent.git") let fork = try await getFork(on: app.db, metadata: metadata) XCTAssertEqual(fork, .parentId(.id0)) } do { // test lookup when package is in the index but with different case in URL metadata.repository?.parent = .init(url: "https://github.com/Foo/Parent.git") let fork = try await getFork(on: app.db, metadata: metadata) XCTAssertEqual(fork, .parentId(.id0)) } do { // test lookup when package is not in the index metadata.repository?.parent = .init(url: "https://github.com/some/other.git") let fork = try await getFork(on: app.db, metadata: metadata) XCTAssertEqual(fork, .parentURL("https://github.com/some/other.git")) } do { // test lookup when parent url is nil metadata.repository?.parent = .init(url: nil) let fork = try await getFork(on: app.db, metadata: metadata) XCTAssertEqual(fork, nil) } }

and then we simply extend a single test where we ensure fork is written to the db in updateRepository. For example, I'd modify test_updateRepository_update and add fork (call on or around line 152):

try await updateRepository(on: app.db, for: repo, metadata: md, licenseInfo: .init(htmlUrl: "license url"), readmeInfo: .init(etag: "etag", html: "readme html", htmlUrl: "readme html url", imagesToCache: []), s3Readme: .cached(s3ObjectUrl: "url", githubEtag: "etag"), fork: .parentURL("https://github.com/foo/bar.git"))

and validate it (around line 168):

XCTAssertEqual(repo.forkedFrom, .parentURL("https://github.com/foo/bar.git"))

finestructure · 2024-09-05T08:14:44Z

LGTM! Thanks for the review and fixes. @finestructure could you link the logic for normalizing the repo urls from PackageList? I couldn't find it.

It's here: https://github.com/SwiftPackageIndex/PackageList/blob/08b5021396a618f2c596dc6f48c336c82901d966/.github/add_package.swift#L24

This is essentially what determines the normalisation in the packages.url field when we add packages.

supersonicbyte · 2024-09-05T08:17:00Z

LGTM! Thanks for the review and fixes. @finestructure could you link the logic for normalizing the repo urls from PackageList? I couldn't find it.

It's here: https://github.com/SwiftPackageIndex/PackageList/blob/08b5021396a618f2c596dc6f48c336c82901d966/.github/add_package.swift#L24

This is essentially what determines the normalisation in the packages.url field when we add packages.

Thanks! That could be definitely reused.

cla-bot · 2024-09-05T12:44:49Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Ucanbarlic.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

supersonicbyte · 2024-09-05T12:49:03Z

Hey @finestructure I incorporated your suggestions, please take a look when u find the time! I also took the URL approach from PackageList, it seems nicer than the way I did it.

finestructure

Looks good! Only a couple of small things left that are essentially changes to my previously proposed changes.

finestructure · 2024-09-06T07:36:13Z

Sources/App/Commands/Ingest.swift

+
+                    let fork: Fork?
+                    do {
+                        fork = try await getFork(on: database, metadata: metadata)
+                    } catch {
+                        fork = nil
+                        Current.logger().warning("updating forked from failed")
+                    }


Suggested change

let fork: Fork?

do {

fork = try await getFork(on: database, metadata: metadata)

} catch {

fork = nil

Current.logger().warning("updating forked from failed")

}

let fork = try? await getFork(on: database, metadata: metadata)

Let's just straight up assign it. getFork is technically throwing due to query but we could even write try? await Package.query there, to be honest.

In fact, I think

func getFork(on database: Database, metadata: Github.Metadata) async -> Fork? {

is better, because we're mixing using nil as an "error" value and throws at the same time - my bad!

Also note that I'd left another little tweak to getFork in a follow up comment on the other PR #3355 (comment)

So in total it should be

func getFork(on database: Database, parent: Github.Metadata.Parent?) async -> Fork? { guard let parentUrl = parent?.normalizedURL else { return nil } if let packageId = try? await Package.query(on: database) .filter(\.$url, .custom("ilike"), parentUrl) .first()?.id { return .parentId(packageId) } else { return .parentURL(parentUrl) } }

Sorry for the back and forth!

finestructure · 2024-09-06T07:40:33Z

Sources/App/Commands/Ingest.swift

    try await repository.save(on: database)
 }

+func getFork(on database: Database, metadata: Github.Metadata) async throws -> Fork? {


Suggested change

func getFork(on database: Database, metadata: Github.Metadata) async throws -> Fork? {

func getFork(on database: Database, metadata: Github.Metadata) async -> Fork? {

finestructure · 2024-09-06T07:40:46Z

Sources/App/Commands/Ingest.swift

+        return nil
+    }
+
+    if let packageId = try await Package.query(on: database)


Suggested change

if let packageId = try await Package.query(on: database)

if let packageId = try? await Package.query(on: database)

finestructure · 2024-09-06T07:43:58Z

Sources/App/Commands/Ingest.swift

 }
+
+private extension URL {
+    var normalizedParent: Self? {


Suggested change

var normalizedParent: Self? {

var normalized: Self? {

as this is for URLs generally.

And I think we should drop the private and put it in a new file URL+ext.swift in Core/Extensions.

supersonicbyte · 2024-09-06T12:21:12Z

@finestructure thanks for the feedback! Here we go again, could you please review again? Thanks!

finestructure · 2024-09-07T10:34:52Z

Sources/App/Commands/Ingest.swift

                        s3Readme = .error("\(error)")
                    }
+
+                    let fork: Fork? = try? await getFork(on: database, parent: metadata.repository?.parent)


There's a warning here, I'll fix it up real quick.

daveverwer requested a review from finestructure September 5, 2024 06:22

cla-bot bot added the cla-signed label Sep 5, 2024

daveverwer requested a review from supersonicbyte September 5, 2024 06:29

finestructure reviewed Sep 5, 2024

View reviewed changes

supersonicbyte approved these changes Sep 5, 2024

View reviewed changes

finestructure requested changes Sep 5, 2024

View reviewed changes

cla-bot bot removed the cla-signed label Sep 5, 2024

supersonicbyte force-pushed the add-forked-from branch from 226794d to e69091c Compare September 5, 2024 12:46

cla-bot bot added the cla-signed label Sep 5, 2024

supersonicbyte requested a review from finestructure September 5, 2024 12:48

finestructure requested changes Sep 6, 2024

View reviewed changes

supersonicbyte requested a review from finestructure September 6, 2024 12:21

finestructure reviewed Sep 7, 2024

View reviewed changes

finestructure approved these changes Sep 7, 2024

View reviewed changes

supersonicbyte and others added 10 commits September 7, 2024 12:40

Implement forked from feature

8b25fd2

Move forked from logic to updateRepository method

301bb2d

Make filter case insensitive

315bf1b

Clean up.

b2d338a

I don’t believe parent is a required field.

75e2ed3

Sort fields

91ed579

Rename migration to match field name

41823a0

Move out to a seperate method

908963f

Resolve PR comments

0c04c1a

Fix warning, tidy up test

21d2281

finestructure force-pushed the add-forked-from branch from 6a6a79a to 21d2281 Compare September 7, 2024 10:40

finestructure enabled auto-merge September 7, 2024 10:40

finestructure merged commit 71387f5 into main Sep 7, 2024
5 checks passed

finestructure deleted the add-forked-from branch September 7, 2024 11:02

finestructure mentioned this pull request Sep 9, 2024

Add Forked From #3355

Closed

	func getFork(on database: Database, metadata: Github.Metadata) async throws -> Fork? {
	func getFork(on database: Database, metadata: Github.Metadata) async -> Fork? {

	if let packageId = try await Package.query(on: database)
	if let packageId = try? await Package.query(on: database)

Uh oh!

Add forked_from as a pointer either to an existing package or as a reference to a URL #3363

Add forked_from as a pointer either to an existing package or as a reference to a URL #3363

Uh oh!

Conversation

daveverwer commented Sep 5, 2024

Uh oh!

finestructure commented Sep 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

supersonicbyte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

finestructure commented Sep 5, 2024

Uh oh!

supersonicbyte commented Sep 5, 2024

Uh oh!

cla-bot bot commented Sep 5, 2024

Uh oh!

supersonicbyte commented Sep 5, 2024

Uh oh!

finestructure left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

supersonicbyte commented Sep 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Add `forked_from` as a pointer either to an existing package or as a reference to a URL #3363

Add `forked_from` as a pointer either to an existing package or as a reference to a URL #3363